fix: handle TsTypeAssertion that conflicts with JSX handling for tarball bundling#7049
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR makes multiple edge-bundler changes: replace direct acorn.parse calls with a new parseAST that tries JSX parsing and retries without JSX on SyntaxError; add a Vitest for the TypeScript type-assertion vs JSX parsing edge case; add WASM magic-byte detection and a shouldRewrite check in tarball import-assertion rewriting with safer error handling; add a deno_dom WASM fixture and tarball inspect template updates; adjust bundler tests and bump the CI Deno matrix to v2.8.0. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // We want to make sure we can handle this case and still rewrite the import assertions correctly. | ||
| const source = ` | ||
| import data3 from './data.json' assert { type: 'json' }; | ||
| const params = <Params> inputs; |
There was a problem hiding this comment.
This is valid and documented syntax - https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#type-assertions - it just conflicts in JSX mode because angled brackets become ambiguous so parsers intentionally disable handling of this syntax for type casting in JSX mode
| const parseAST = (source: string): Program => { | ||
| try { | ||
| return acornJSX.parse(source, parseOptions) | ||
| } catch (error) { | ||
| // for non-jsx typescript casting to type via "<type> value" (normally done with "value as type") will throw an "Unexpected token" error in acorn-jsx, | ||
| // but is valid syntax in TypeScript. In this case, we can retry parsing with the non-jsx parser. | ||
| if (error instanceof SyntaxError) { | ||
| return acornNoJSX.parse(source, parseOptions) | ||
| } | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential alternative I considered is picking parser based on extension (with .ts specifically using NoJSX mode), but fallback felt like more sure thing that is potentially not susceptible to wild and unexpected configurations
pieh
force-pushed
the
fix/handle-TsTypeAssertion-when-rewriting-import-assertions
branch
2 times, most recently
from
75fb8b8 to
0360098
Compare
pieh
force-pushed
the
fix/handle-TsTypeAssertion-when-rewriting-import-assertions
branch
from
0360098 to
9d8b483
Compare
| .replace(/file:\/\/\/(.*?\/)(build\/packages\/edge-bundler\/deno\/vendor\/deno\.land\/x\/eszip.*)/, 'file://$2') | ||
| // eslint-disable-next-line no-control-regex | ||
| .replace(/[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g, ''), | ||
| .replace(/[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g, '') | ||
| .replace( | ||
| /file:\/\/\/(.*?\/)(build\/packages\/edge-bundler\/deno\/vendor\/deno\.land\/x\/eszip.*)/, | ||
| 'file://$2', | ||
| ), |
There was a problem hiding this comment.
swap places so formatting removal is before path normalization. Seems like some newer deno version messed with formatting so previous order was not working anymore and then path normalization was not being applied
| // The vendored deno_dom WASM payload must be present in the tarball. | ||
| // Deno <2.6 vendors `.wasm` imports under a `.d.mts` extension (with a | ||
| // content-hash suffix); 2.6+ keeps the original `.wasm` extension. | ||
| const denoDomVendorPrefix = './vendor/deno.land/x/deno_dom@v0.1.56/build/deno-wasm/' | ||
| const expectedWasmEntry = lt(denoVersion, '2.6.0') | ||
| ? `${denoDomVendorPrefix}#deno-wasm_bg.wasm_d2792.d.mts` | ||
| : `${denoDomVendorPrefix}deno-wasm_bg.wasm` | ||
| expect(entries).toContain(expectedWasmEntry) |
There was a problem hiding this comment.
I don't love check like this - bit I added it here to somewhat show that wasm handling is deno-version dependant and give some more context around added byte sniffing for .ts extension that should be text/code and not a wasm binary
| const entrypoint = path.resolve(manifest.functions[functionName]); | ||
| const func = await import(pathToFileURL(entrypoint)) | ||
| // Import via a relative specifier so Deno resolves the module URL itself, | ||
| // keeping its encoding consistent with the import map base (both derived from | ||
| // cwd). Pre-building an absolute file:// URL on the Node side encodes paths | ||
| // differently from Deno (e.g. '~' in Windows 8.3 short names), which breaks | ||
| // import map prefix matching on Deno 2.8+. | ||
| const func = await import("./" + manifest.functions[functionName]); |
There was a problem hiding this comment.
This did not work on Windows Github Actions runner with fresh deno - overall it looks like windows quirk. It worked fine on non-windows, so this doesn't seem like something to be concerned about actual EF runtime
| deno-version: ['v2.4.2'] | ||
| # Maximum supported deno version from the `DENO_VERSION_RANGE` constant in `node/bridge.ts`. | ||
| # If there is no upper band - this should be set to whatever is the latest deno version at the time of updating this workflow. | ||
| deno-version: ['v2.8.0'] |
There was a problem hiding this comment.
Additional side effect is that github commit checks have different names, so "required checks" will need to be updated before merging.
Fact that currently set ones did not run is result of this change.
We do use min deno version in variant below, so that we have min and max deno version cases covered
pieh
deleted the
fix/handle-TsTypeAssertion-when-rewriting-import-assertions
branch
2 participants
🎉 Thanks for submitting a pull request! 🎉
Summary
Primary motivation here is not NOT fail bundling when
.wasmimport happens + handle currently unsupported<Type> variablesyntax for casting.For the
.wasmhandling important note is that we are NOT actually using the wasm imported module in runtime as of now and this is only to unblock bundling. We should be falling back to pre deno@2.1 handling (so either fetching or instantiating wasm module from inline bytes).The fact that deno@2.6.0 changed the way vendored
.wasmmodules are being handled is a bit problematic. It does mean that if we will want to actually support them (instead of just pre deno@2.1 way) - we will need to keep the difference in mind - we will either need to ensure that runtime version matches deno version used for bundling (at least the pre/post 2.6.0 ranges, it probably won't have to match exactly) OR runtime will need to somehow provide compatibility layer to adjust bundle so mismatched wasm handling would be addressedFor us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)